Fix wshp array bounds error and sizing #10735

mjwitte · 2024-09-13T02:31:02Z

Pull request overview

Fixes WaterToAirHeatPump:EquationFit sizing companion coil array bounds error #9849
Fixes Coil:Heating:WaterToAirHeatPump:EquationFit zero Rated Water Flow Rate when coil capacity is hard-sized.

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

Reviewer

This will not be exhaustively relevant to every PR.

…9wshpArrayBounds

rraustad · 2024-09-19T03:36:25Z

src/EnergyPlus/WaterToAirHeatPumpSimple.cc

@@ -2580,86 +2580,92 @@ namespace WaterToAirHeatPumpSimple {

                // determine adjusted cooling and heating coil capacity
                simpleWatertoAirHP.RatedCapHeatAtRatedCdts = RatedCapHeatDes * RatedHeatCapTempModFac;


I believe what the code below is trying to do is to size a HP, so the coils would need to be the same size. In this case that's not necessary but an actual HP would need to be tested to make sure it works. I guess it does work since all WSHPs apparently size correctly, or do they?

rraustad · 2024-09-19T03:37:08Z

src/EnergyPlus/UnitarySystem.cc

+                    state.dataSize->DataConstantUsedForSizing = WaterToAirHeatPumpSimple::GetCoilCapacity(
+                        state, HVAC::cAllCoilTypes(this->m_CoolingCoilType_Num), this->m_CoolingCoilName, ErrFound);
+                    EqSizing.DesCoolingLoad = state.dataSize->DataConstantUsedForSizing;
+                }


I just wrapped all this in 1 IF block. Still not sure if the code at the end of this block is needed.

The next step is to remove this IF block. There is no reason to call the cooling coil to report the UnitarySystem heating capacity. That's a cover up.

rraustad · 2024-09-19T13:06:28Z

Regarding if this fix is correct, there is the same check for the cooling coil where the companionCoil index is checked before looking for the companion heating coil. So this does look like the fix moved in the right direction. It's just that there are some oddities that could also be looked at here or in a separate issue. See issue discussion.

rraustad · 2024-09-19T13:13:07Z

Also some interesting table reports.

9849-Solution2-Separate UnitarySystems-in-v24.2-CompanionCoilCrash.txt

rraustad · 2024-09-19T17:46:52Z

src/EnergyPlus/WaterToAirHeatPumpSimple.cc

@@ -2734,7 +2740,7 @@ namespace WaterToAirHeatPumpSimple {

                // user provided inputs are assumed to be at rated conditions
                simpleWatertoAirHP.RatedPowerHeat = simpleWatertoAirHP.RatedCapHeat / simpleWatertoAirHP.RatedCOPHeatAtRatedCdts;
-                simpleWatertoAirHP.RatedCapHeatAtRatedCdts = 0;
+                simpleWatertoAirHP.RatedCapHeatAtRatedCdts = simpleWatertoAirHP.RatedCapHeat;


Why would the rated capacity sizing variable be set to 0 if not autosized? And what about rated power below? and check the cooling coil code.

Real64 RatedCapHeatAtRatedCdts = 0.0; // Rated Heating Capacity at Rated Conditions [W]

I guess I could have changed the water flow rate sizing to use RatedCapHeat instead.

if (simpleWatertoAirHP.WAHPType == WatertoAirHP::Heating) { RatedWaterVolFlowRateDes = simpleWatertoAirHP.RatedCapHeatAtRatedCdts / (state.dataSize->PlantSizData(PltSizNum).DeltaT * Cp * rho);

There is likely a difference between RatedCapHeat and RatedCapHeatAtRatedCdts given the CapFT term applied. This is above at line 2743.

simpleWatertoAirHP.RatedCapHeat = RatedCapHeatDes; simpleWatertoAirHP.RatedCapHeatAtRatedCdts = RatedCapHeatDes * RatedHeatCapTempModFac;

I think these equations need to be moved up into the IF blocks in order to use the correct variable. If RatedCapHeat is the coil load and RatedCapHeatatRatedCdts is the coil size, when autosized, then declaring these variables AFTER the coil size is determined seems wrong (i.e., after the if/else). If autosized, RatedCapHeatatRatedCdts should be used as the capacity, if not autosized, RatedCapHeat should be used as the capacity.

I actually want to revert this change and change the RatedWaterVolFlowRateDes sizing equation to use RatedCapHeat because that's what the sizing routine is setting as the final coil capacity. And the calc routine uses RatedCapHeat as the coil capacity. So RatedCapHeat should be the final coil size whether the coil is autosized or hard-sized.

HeatCapAtPeak = rhoair * VolFlowRate * Psychrometrics::PsyCpAirFnW(DataPrecisionGlobals::constant_zero) * (HeatSupTemp - HeatMixTemp); // heating coil load RatedCapHeatDes = (PeakHeatCapTempModFac > 0.0) ? HeatCapAtPeak / PeakHeatCapTempModFac : HeatCapAtPeak; // heating capacity final determination simpleWatertoAirHP.RatedCapHeat = RatedCapHeatDes; simpleWatertoAirHP.RatedCapHeatAtRatedCdts = RatedCapHeatDes * RatedHeatCapTempModFac; void CalcHPHeatingSimple(EnergyPlusData &state, HeatCapRated = simpleWatertoAirHP.RatedCapHeat;

So it follows that the water flow rate should use RatedHeatCap because that's the coil size. You can't see an issue in model performance when the water flow rate is sized with either of these variables because only exit water temp changes, not performance. You would have to really critique the exiting water temp to see an issue and even then it would be hard to see.

if (simpleWatertoAirHP.WAHPType == WatertoAirHP::Heating) { RatedWaterVolFlowRateDes = simpleWatertoAirHP.RatedCapHeatAtRatedCdts / (state.dataSize->PlantSizData(PltSizNum).DeltaT * Cp * rho);

And then I look at this line and it looks like RatedCapHeatAtRatedCdts is the correct variable to use to size the water flow rate. So I will leave this branch in this state for now even though I think it's wrong.

RatedHeatCapTempModFac = Curve::CurveValue(state, simpleWatertoAirHP.HeatCapCurveIndex, RatedHeatratioTDB, RatedHeatratioTS, 1.0, 1.0);

RatedCapHeatDes is the CapFT modified coil capacity, as is RatedCapHeat. The next line modifies this value by the rated CapFT, which should be 1 by definition (but I expect it's not 1). So now I'm back to using RatedCapHeat to size the water flow rate. This is so very confusing but I think I am more comfortable with this now. @mjwitte ?

simpleWatertoAirHP.RatedCapHeatAtRatedCdts = RatedCapHeatDes * RatedHeatCapTempModFac;

…lures

…9wshpArrayBounds

rraustad · 2024-09-20T03:41:31Z

@mjwitte I think it's time to review changes in unmet hours. This is for 9849-OffSml-MultiHVACSys DOAS_EP9_4 to24.2.

rraustad · 2024-09-20T04:05:12Z

src/EnergyPlus/WaterToAirHeatPumpSimple.cc

                        } else {
                            SystemCapacity = companionHeatingCoil.RatedCapHeat;
                        }
                    } else {
-                        SystemCapacity = simpleWatertoAirHP.RatedCapCoolAtRatedCdts;
+                        SystemCapacity = simpleWatertoAirHP.RatedCapCoolAtRatedCdts; // RatedCapCoolTotal ? * (1 + 1/COP) ?


These should also include the sizing ratio when basing condenser water flow sizing on cooling capacity.

AirLoopHVAC:UnitarySystem, N1 , \field DX Heating Coil Sizing Ratio

I'm not awake yet apparently. I read the end comment here as actual code and could not understand what the ternary with multiplier (? *) was doing 😴

github-actions · 2024-09-20T04:18:41Z

⚠️ Regressions detected on macos-14 for commit `f047836`

Regression Summary

EIO: 14
MTR Small Diffs: 6
Table Big Diffs: 5
ESO Small Diffs: 14
Table Small Diffs: 9
Table String Diffs: 2

nrel-bot · 2024-10-18T00:20:37Z

@mjwitte @Myoldmopar it has been 28 days since this pull request was last updated.

nealkruis · 2024-11-01T15:15:39Z

@mjwitte, @rraustad checking in on this PR. I believe this is critical for CBECC.

…rayBounds

mjwitte · 2024-11-04T22:26:47Z

@rraustad Are you satisfied with the current changes? Defect file OffSml-MultiHVACSys DOAS_EP9_4 to24.2 hours not met look good now.

Develop:

This branch:

Version 9.4:

rraustad · 2024-11-04T22:37:49Z

@mjwitte I was looking over this last night. I still have questions on the best way to size this model but without a defect file that shows an issue I would rather not guess based on code review. So with no diffs I guess this can be merged so that additional testing can help with finding issues. @nealkruis would there be someone available to run this against known good input files?

mjwitte · 2024-11-04T22:46:12Z

@mjwitte I was looking over this last night. I still have questions on the best way to size this model but without a defect file that shows an issue I would rather not guess based on code review. So with no diffs I guess this can be merged so that additional testing can help with finding issues. @nealkruis would there be someone available to run this against known good input files?

\EnergyPlusDevSupport\DefectFiles\9000s\9849\Ticket 16738\OffSml-MultiHVACSys DOAS_EP9_4 to24.2.idf should work.
With develop debug, it trips an array bounds error. With develop release there are too many hrs not met. With this branch, results are back in line with v9.4.

@rraustad I have other files from the same user, what combination are you looking for?

rraustad · 2024-11-04T22:49:26Z

I was interested if this change had an impact on sizing that wasn't expected. So a file that worked before should have the same sizing results?

rraustad · 2024-11-04T22:58:20Z

Given the unit tests that check autosized capacity maybe I'm being too cautious. If unit tests and example files have not changed then this is probably good enough.

mjwitte · 2024-11-04T23:20:09Z

Here's a sizing comparison for one of the defect files.
9849 Sizing Results.xlsx

github-actions · 2024-11-04T23:40:37Z

⚠️ Regressions detected on macos-14 for commit `55833ce`

Regression Summary

EIO: 14
MTR Small Diffs: 6
Table Big Diffs: 5
ESO Small Diffs: 14
Table Small Diffs: 9
Table String Diffs: 2

rraustad · 2024-11-05T00:05:20Z

Those results look good. The blank results that show up now are likely related to #9273 moving PTUnits to UnitarySystem. Since design size cooling capacity reports correctly it's probably a simple fix to get design size heating capacity to also report.

rraustad · 2024-11-05T14:29:55Z

This would not be an easy fix. The UnitarySystem is pre-determining the operating air flow and capacity and passing that information to the coils. It appears to coil sizing as if the coils were hard-sized, which they were in this case. I've seen this before. For these single coil UnitarySystems the zone air flow rate is more than 10% different so should be reporting both design size and user specified values. Showing the air flow rates here as an example, similar issue with coil capacity.

Zone Sizing Information, PERIMZN_1, Cooling, 6090.56736, 6090.56736, 0.44620, 0.44620
Zone Sizing Information, PERIMZN_1, Heating, 5721.55938, 3803.13801, 0.33564, 0.22310

Component Sizing Information, AirLoopHVAC:UnitarySystem, PERIM_1_WSHP_COILCLG UNITARY, User-Specified Cooling Supply Air Flow Rate [m3/s], 0.57633

Component Sizing Information, COIL:COOLING:WATERTOAIRHEATPUMP:EQUATIONFIT, PERIM_1_WSHP_COILCLG, Design Size Rated Air Flow Rate [m3/s], 0.57633
Component Sizing Information, COIL:COOLING:WATERTOAIRHEATPUMP:EQUATIONFIT, PERIM_1_WSHP_COILCLG, User-Specified Rated Air Flow Rate [m3/s], 0.57633

Component Sizing Information, COIL:HEATING:WATERTOAIRHEATPUMP:EQUATIONFIT, PERIM_1_WSHP_COILHTG, Design Size Rated Air Flow Rate [m3/s], 0.57633
Component Sizing Information, COIL:HEATING:WATERTOAIRHEATPUMP:EQUATIONFIT, PERIM_1_WSHP_COILHTG, User-Specified Rated Air Flow Rate [m3/s], 0.57633

This is where the heating coil gets it's air flow value, from the parent. But this is not the autosized air flow rate if the coil air flow is hard sized, it's the value passed from the parent. I think here, if the air flow is hard sized, then the zone or system design air flow rate should be set to the autosized value (e.g., FinalZoneSizing or FinalSysSizing data)

} else if (this->unitarySysEqSizing(this->curSysNum).HeatingAirFlow) {
    this->autoSizedValue = this->unitarySysEqSizing(this->curSysNum).HeatingAirVolFlow;

since the non-autosized value is stored in this->originalValue. Then the sizing could report both if > than 10% different. This is a bigger issue than should be handled in this branch.

} else if (!this->wasAutoSized &&
           (this->autoSizedValue == this->originalValue || this->autoSizedValue == 0.0)) { // no sizing run done or autosizes to 0
    this->autoSizedValue = this->originalValue;
    if (this->dataAutosizable || (!this->sizingDesRunThisZone && Util::SameString(this->compType, "Fan:ZoneExhaust"))) {
        this->reportSizerOutput(
            state, this->compType, this->compName, "User-Specified " + this->sizingStringScalable + this->sizingString, this->autoSizedValue);
    }

rraustad · 2024-11-05T14:32:35Z

@mjwitte this branch corrects the issue and does not appear to break anything. I suggest getting this in and posting another issue to resolve this more difficult logic flow. I'll do that now. See #10811.

mjwitte · 2024-11-07T18:04:31Z

I approve @rraustad 's changes here. @Myoldmopar This is ready to merge.

nealkruis · 2024-11-07T20:04:45Z

@mjwitte, can you confirm that this also resolves the issues in CBECC generated files I sent you? Thanks!

mjwitte · 2024-11-07T21:51:01Z

@mjwitte, can you confirm that this also resolves the issues in CBECC generated files I sent you? Thanks!

Yes, it does.

Myoldmopar · 2024-11-08T14:36:02Z

Thanks for confirming that for @nealkruis , @mjwitte . And for your efforts too, @rraustad . Merging this.

Avoid wshp array bounds

433593a

mjwitte added the Defect Includes code to repair a defect in EnergyPlus label Sep 13, 2024

mjwitte mentioned this pull request Sep 13, 2024

WaterToAirHeatPump:EquationFit sizing companion coil array bounds error #9849

Closed

3 tasks

rraustad added 2 commits September 18, 2024 23:32

Protect companion coil for heating coil in sizing

5289b30

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into 984…

9d3f048

…9wshpArrayBounds

rraustad reviewed Sep 19, 2024

View reviewed changes

Use hard-sized capacity

ef29015

rraustad reviewed Sep 19, 2024

View reviewed changes

rraustad added 2 commits September 19, 2024 23:36

Revert and correct water flow rate, not surprised of no unit test fai…

76d1800

…lures

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into 984…

6e1066c

…9wshpArrayBounds

rraustad reviewed Sep 20, 2024

View reviewed changes

Merge remote-tracking branch 'remotes/origin/develop' into 9849wshpAr…

672c1d4

…rayBounds

mjwitte changed the title ~~Avoid wshp array bounds error~~ Fix wshp array bounds error and sizing Nov 5, 2024

rraustad mentioned this pull request Nov 5, 2024

UnitarySystem component sizing does not report design values when greater than 10% different than user specified (hard-sized) values #10811

Open

3 tasks

mjwitte marked this pull request as ready for review November 5, 2024 14:43

Myoldmopar approved these changes Nov 8, 2024

View reviewed changes

Myoldmopar merged commit 8cf8dc3 into develop Nov 8, 2024
10 checks passed

Myoldmopar deleted the 9849wshpArrayBounds branch November 8, 2024 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix wshp array bounds error and sizing #10735

Fix wshp array bounds error and sizing #10735

mjwitte commented Sep 13, 2024 •

edited

Loading

rraustad Sep 19, 2024

rraustad Sep 19, 2024

rraustad Sep 20, 2024

rraustad commented Sep 19, 2024

rraustad commented Sep 19, 2024 •

edited

Loading

rraustad Sep 19, 2024 •

edited

Loading

rraustad Sep 19, 2024

rraustad Sep 19, 2024 •

edited

Loading

rraustad Sep 19, 2024 •

edited

Loading

rraustad Sep 20, 2024 •

edited

Loading

rraustad Sep 20, 2024

rraustad commented Sep 20, 2024

rraustad Sep 20, 2024

Myoldmopar Nov 8, 2024

github-actions bot commented Sep 20, 2024

nrel-bot commented Oct 18, 2024

nealkruis commented Nov 1, 2024

mjwitte commented Nov 4, 2024

rraustad commented Nov 4, 2024

mjwitte commented Nov 4, 2024

rraustad commented Nov 4, 2024

rraustad commented Nov 4, 2024

mjwitte commented Nov 4, 2024

github-actions bot commented Nov 4, 2024

rraustad commented Nov 5, 2024 •

edited

Loading

rraustad commented Nov 5, 2024

rraustad commented Nov 5, 2024 •

edited

Loading

mjwitte commented Nov 7, 2024

nealkruis commented Nov 7, 2024

mjwitte commented Nov 7, 2024

Myoldmopar commented Nov 8, 2024

		@@ -2580,86 +2580,92 @@ namespace WaterToAirHeatPumpSimple {

		// determine adjusted cooling and heating coil capacity
		simpleWatertoAirHP.RatedCapHeatAtRatedCdts = RatedCapHeatDes * RatedHeatCapTempModFac;

Fix wshp array bounds error and sizing #10735

Fix wshp array bounds error and sizing #10735

Conversation

mjwitte commented Sep 13, 2024 • edited Loading

Pull request overview

Pull Request Author

Reviewer

rraustad Sep 19, 2024

Choose a reason for hiding this comment

rraustad Sep 19, 2024

Choose a reason for hiding this comment

rraustad Sep 20, 2024

Choose a reason for hiding this comment

rraustad commented Sep 19, 2024

rraustad commented Sep 19, 2024 • edited Loading

rraustad Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

rraustad Sep 19, 2024

Choose a reason for hiding this comment

rraustad Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

rraustad Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

rraustad Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

rraustad Sep 20, 2024

Choose a reason for hiding this comment

rraustad commented Sep 20, 2024

rraustad Sep 20, 2024

Choose a reason for hiding this comment

Myoldmopar Nov 8, 2024

Choose a reason for hiding this comment

github-actions bot commented Sep 20, 2024

⚠️ Regressions detected on macos-14 for commit f047836

nrel-bot commented Oct 18, 2024

nealkruis commented Nov 1, 2024

mjwitte commented Nov 4, 2024

rraustad commented Nov 4, 2024

mjwitte commented Nov 4, 2024

rraustad commented Nov 4, 2024

rraustad commented Nov 4, 2024

mjwitte commented Nov 4, 2024

github-actions bot commented Nov 4, 2024

⚠️ Regressions detected on macos-14 for commit 55833ce

rraustad commented Nov 5, 2024 • edited Loading

rraustad commented Nov 5, 2024

rraustad commented Nov 5, 2024 • edited Loading

mjwitte commented Nov 7, 2024

nealkruis commented Nov 7, 2024

mjwitte commented Nov 7, 2024

Myoldmopar commented Nov 8, 2024

mjwitte commented Sep 13, 2024 •

edited

Loading

rraustad commented Sep 19, 2024 •

edited

Loading

rraustad Sep 19, 2024 •

edited

Loading

rraustad Sep 19, 2024 •

edited

Loading

rraustad Sep 19, 2024 •

edited

Loading

rraustad Sep 20, 2024 •

edited

Loading

⚠️ Regressions detected on macos-14 for commit `f047836`

⚠️ Regressions detected on macos-14 for commit `55833ce`

rraustad commented Nov 5, 2024 •

edited

Loading

rraustad commented Nov 5, 2024 •

edited

Loading